Wrap cursor around viewport during G/R/S (#3255) - #4486
Conversation
|
Cool! Could you post a video of it in action? Ideally on both desktop and web. That'd help to prioritize the review. |
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Confidence score: 2/5
editor/src/messages/app_window/app_window_message_handler.rscan process each webpointermovethrough both absolute and relative paths, causing the absolute update to overwrite relative G/R/S motion and likely breaking pointer-locked transforms — ensure only one path handles locked events.editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rsadds raw physical deltas to viewport-logical coordinates, so desktop or scaled-device input may produce incorrect movement — normalize the delta into the coordinate space used byself.mouse.position.frontend/wrapper/src/editor_commands.rscontains an unusedapp_window_pointer_unlockbinding; this is low risk and appears to be dead code because unlocking is handled by the transform layer — remove it or wire it to the actual frontend path.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rs">
<violation number="1" location="editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rs:111">
P2: The `RelativePointerMove` handler adds the raw physical-device pointer-lock delta directly to `self.mouse.position`, which is in viewport-logical coordinates used by the transform math. If the source (desktop `WindowPointerLockMove` / web `movementX`/`movementY`) does not divide the delta by the viewport/device-pixel scale before emitting it, G/R/S drag speed and distance will be wrong at non-1x HiDPI scale factors. The `AppWindowMessage::PointerLockMove` caller passes `x,y` verbatim with a comment saying the divide is handled at the source, but that contract is only enforced by the platform frontends; the editor-side handler here has no way to detect or correct a scaled delta. Confirm the scaling contract is actually applied (or normalize by `viewport.scale` here) so the accumulated viewport position stays consistent with regular `PointerMove` position updates.</violation>
</file>
<file name="frontend/wrapper/src/editor_commands.rs">
<violation number="1" location="frontend/wrapper/src/editor_commands.rs:87">
P3: The `app_window_pointer_unlock` editor command is never called from the frontend, so its generated `editor.appWindowPointerUnlock()` binding is dead code. Pointer unlock is handled instead by the transform layer directly emitting `AppWindowMessage::PointerUnlock` internally (`transform_layer_message_handler.rs`), so this JS-facing command has no caller. Remove it unless the frontend is intended to call it.</violation>
</file>
<file name="editor/src/messages/app_window/app_window_message_handler.rs">
<violation number="1" location="editor/src/messages/app_window/app_window_message_handler.rs:26">
P1: On web, each locked `pointermove` reaches the editor through both the existing absolute path and this new relative path. The absolute update resets the position that the relative update advances, so G/R/S motion can cancel or jump; suppress normal pointer forwarding during software-cursor pointer lock, or use only one movement path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Also feed relative delta into InputPreprocessor for G/R/S infinite drag (fake cursor) | ||
| // Divide by viewport scale will be handled at source (desktop physical -> logical); here we keep raw | ||
| // but transform_layer will handle scaling via document_to_viewport | ||
| responses.add(InputPreprocessorMessage::RelativePointerMove { delta: glam::DVec2::new(x, y) }); |
There was a problem hiding this comment.
P1: On web, each locked pointermove reaches the editor through both the existing absolute path and this new relative path. The absolute update resets the position that the relative update advances, so G/R/S motion can cancel or jump; suppress normal pointer forwarding during software-cursor pointer lock, or use only one movement path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/app_window/app_window_message_handler.rs, line 26:
<comment>On web, each locked `pointermove` reaches the editor through both the existing absolute path and this new relative path. The absolute update resets the position that the relative update advances, so G/R/S motion can cancel or jump; suppress normal pointer forwarding during software-cursor pointer lock, or use only one movement path.</comment>
<file context>
@@ -14,8 +14,16 @@ impl MessageHandler<AppWindowMessage, ()> for AppWindowMessageHandler {
+ // Also feed relative delta into InputPreprocessor for G/R/S infinite drag (fake cursor)
+ // Divide by viewport scale will be handled at source (desktop physical -> logical); here we keep raw
+ // but transform_layer will handle scaling via document_to_viewport
+ responses.add(InputPreprocessorMessage::RelativePointerMove { delta: glam::DVec2::new(x, y) });
}
AppWindowMessage::Close => {
</file context>
| responses.add(InputMapperMessage::WheelScroll); | ||
| } | ||
| InputPreprocessorMessage::RelativePointerMove { delta } => { | ||
| self.mouse.position += *delta; |
There was a problem hiding this comment.
P2: The RelativePointerMove handler adds the raw physical-device pointer-lock delta directly to self.mouse.position, which is in viewport-logical coordinates used by the transform math. If the source (desktop WindowPointerLockMove / web movementX/movementY) does not divide the delta by the viewport/device-pixel scale before emitting it, G/R/S drag speed and distance will be wrong at non-1x HiDPI scale factors. The AppWindowMessage::PointerLockMove caller passes x,y verbatim with a comment saying the divide is handled at the source, but that contract is only enforced by the platform frontends; the editor-side handler here has no way to detect or correct a scaled delta. Confirm the scaling contract is actually applied (or normalize by viewport.scale here) so the accumulated viewport position stays consistent with regular PointerMove position updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rs, line 111:
<comment>The `RelativePointerMove` handler adds the raw physical-device pointer-lock delta directly to `self.mouse.position`, which is in viewport-logical coordinates used by the transform math. If the source (desktop `WindowPointerLockMove` / web `movementX`/`movementY`) does not divide the delta by the viewport/device-pixel scale before emitting it, G/R/S drag speed and distance will be wrong at non-1x HiDPI scale factors. The `AppWindowMessage::PointerLockMove` caller passes `x,y` verbatim with a comment saying the divide is handled at the source, but that contract is only enforced by the platform frontends; the editor-side handler here has no way to detect or correct a scaled delta. Confirm the scaling contract is actually applied (or normalize by `viewport.scale` here) so the accumulated viewport position stays consistent with regular `PointerMove` position updates.</comment>
<file context>
@@ -107,6 +107,11 @@ impl<'a> MessageHandler<InputPreprocessorMessage, InputPreprocessorMessageContex
responses.add(InputMapperMessage::WheelScroll);
}
+ InputPreprocessorMessage::RelativePointerMove { delta } => {
+ self.mouse.position += *delta;
+
+ responses.add(InputMapperMessage::PointerMove);
</file context>
| AppWindowMessage::PointerLock.into() | ||
| } | ||
|
|
||
| fn app_window_pointer_unlock() -> Message { |
There was a problem hiding this comment.
P3: The app_window_pointer_unlock editor command is never called from the frontend, so its generated editor.appWindowPointerUnlock() binding is dead code. Pointer unlock is handled instead by the transform layer directly emitting AppWindowMessage::PointerUnlock internally (transform_layer_message_handler.rs), so this JS-facing command has no caller. Remove it unless the frontend is intended to call it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/wrapper/src/editor_commands.rs, line 87:
<comment>The `app_window_pointer_unlock` editor command is never called from the frontend, so its generated `editor.appWindowPointerUnlock()` binding is dead code. Pointer unlock is handled instead by the transform layer directly emitting `AppWindowMessage::PointerUnlock` internally (`transform_layer_message_handler.rs`), so this JS-facing command has no caller. Remove it unless the frontend is intended to call it.</comment>
<file context>
@@ -84,6 +84,14 @@ mod editor_commands {
AppWindowMessage::PointerLock.into()
}
+ fn app_window_pointer_unlock() -> Message {
+ AppWindowMessage::PointerUnlock.into()
+ }
</file context>
ac425ab to
ffcfbe3
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
200f879 to
784a670
Compare
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 14 files (changes from recent commits).
Confidence score: 5/5
frontend/wrapper/src/editor_commands.rsaddsapp_window_pointer_unlock(appWindowPointerUnlock) without any frontend callers, so the wrapper currently has no observable effect; add a call site or remove the unused export.
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
d2bf22f to
9848503
Compare
Route pointer-lock deltas through the transform layer so only G/R/S consumes them, keep the wrapped position across chained operations, wrap the software cursor at the viewport edges, and cancel the transform when pointer lock ends. Re-emit the active tool's cursor on unlock.
9848503 to
9f5bb2f
Compare
|
@timon-schelling |
There was a problem hiding this comment.
2 issues found across 13 files
Confidence score: 3/5
- In
frontend/src/components/panels/Document.svelte, requesting pointer lock afterawait tick()can fail on browsers that require transient user activation, preventing web G/R/S interactions from acquiring pointer lock; request it synchronously from the initiating user event and handle the failed transition. - In
desktop/src/app.rs, losing focus during a native number-input drag releases the OS grab without notifyingNumberInputto perform drag cleanup; because native mode does not receive browser pointer-lock events, ensure the focus-loss path explicitly triggers the required cleanup.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/panels/Document.svelte">
<violation number="1" location="frontend/src/components/panels/Document.svelte:561">
P1: On browsers requiring transient user activation, web G/R/S cannot acquire pointer lock because this request runs after `await tick()`. Handle the request synchronously from the initiating user event and abort the transform when the request rejects or emits `pointerlockerror`; otherwise the software cursor remains visible while no movement deltas arrive.</violation>
</file>
<file name="desktop/src/app.rs">
<violation number="1" location="desktop/src/app.rs:588">
P2: When focus is lost during a native number-input drag, this releases the OS grab but does not notify `NumberInput` to run its drag cleanup. Native mode does not use browser pointer-lock events, and the synthesized Escape is sent only to the backend. Send an explicit frontend cancellation/unlock notification or otherwise invoke the native drag cleanup on focus loss.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Browsers reject a re-lock request shortly after an unlock, so retry on each update | ||
| if (data.visible && viewport && window.document.pointerLockElement !== viewport) { | ||
| try { | ||
| viewport.requestPointerLock?.().catch(() => undefined); |
There was a problem hiding this comment.
P1: On browsers requiring transient user activation, web G/R/S cannot acquire pointer lock because this request runs after await tick(). Handle the request synchronously from the initiating user event and abort the transform when the request rejects or emits pointerlockerror; otherwise the software cursor remains visible while no movement deltas arrive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/panels/Document.svelte, line 561:
<comment>On browsers requiring transient user activation, web G/R/S cannot acquire pointer lock because this request runs after `await tick()`. Handle the request synchronously from the initiating user event and abort the transform when the request rejects or emits `pointerlockerror`; otherwise the software cursor remains visible while no movement deltas arrive.</comment>
<file context>
@@ -520,6 +545,31 @@
+ // Browsers reject a re-lock request shortly after an unlock, so retry on each update
+ if (data.visible && viewport && window.document.pointerLockElement !== viewport) {
+ try {
+ viewport.requestPointerLock?.().catch(() => undefined);
+ } catch {
+ // Retried on the next update
</file context>
| && self.input_state.pointer_locked() | ||
| { | ||
| self.unlock_pointer(); | ||
| self.send_cancel_escape(); |
There was a problem hiding this comment.
P2: When focus is lost during a native number-input drag, this releases the OS grab but does not notify NumberInput to run its drag cleanup. Native mode does not use browser pointer-lock events, and the synthesized Escape is sent only to the backend. Send an explicit frontend cancellation/unlock notification or otherwise invoke the native drag cleanup on focus loss.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/app.rs, line 588:
<comment>When focus is lost during a native number-input drag, this releases the OS grab but does not notify `NumberInput` to run its drag cleanup. Native mode does not use browser pointer-lock events, and the synthesized Escape is sent only to the backend. Send an explicit frontend cancellation/unlock notification or otherwise invoke the native drag cleanup on focus loss.</comment>
<file context>
@@ -539,20 +570,22 @@ impl ApplicationHandler for App {
+ && self.input_state.pointer_locked()
+ {
+ self.unlock_pointer();
+ self.send_cancel_escape();
}
</file context>
…ge_handler.rs Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Confidence score: 3/5
- In
editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs, a PTZ change during a drag can advance the software cursor while the early return skips the G/R/S arms, leaving the object without the corresponding delta; ensure the active transform also receives that movement before returning. - The duplicated software-cursor update logic in
editor/src/messages/tool/transform_layer/transform_layer_message_handler.rsincreases maintenance risk and could let the two paths diverge; extract the shared delta, wrapping, and update emission into a helper.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs">
<violation number="1" location="editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs:543">
P3: This software-cursor advance (delta accumulation, wrap, UpdateSoftwareCursor emit) is now duplicated verbatim in the PTZ-change branch and the main path. Extract it into a small helper (e.g. `fn update_software_cursor(&mut self, target: ViewportPosition, responses: &mut VecDeque<Message>)` that computes the delta from `self.mouse_position`) and call it from both paths so the two copies cannot drift apart again.</violation>
<violation number="2" location="editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs:543">
P3: When PTZ changes mid-gesture (e.g., wheel-zoom while dragging), this branch moves the software cursor by `delta` but the early `return` skips the G/R/S arms, so the object never gets that `delta`. The cursor (and the final hand-off of the grab) then stays offset from the transformed layer by the dropped deltas. Since the pre-existing code dropped the delta entirely on PTZ change, it either needs to apply the delta to the G/R/S operation as well (moving the return to after the transform arms), or not move the cursor here — otherwise the two paths diverge.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| self.ptz = document.document_ptz; | ||
| if old_ptz != self.ptz { | ||
| self.mouse_position = input.mouse.position; | ||
| if self.software_cursor_active { |
There was a problem hiding this comment.
P3: This software-cursor advance (delta accumulation, wrap, UpdateSoftwareCursor emit) is now duplicated verbatim in the PTZ-change branch and the main path. Extract it into a small helper (e.g. fn update_software_cursor(&mut self, target: ViewportPosition, responses: &mut VecDeque<Message>) that computes the delta from self.mouse_position) and call it from both paths so the two copies cannot drift apart again.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs, line 543:
<comment>This software-cursor advance (delta accumulation, wrap, UpdateSoftwareCursor emit) is now duplicated verbatim in the PTZ-change branch and the main path. Extract it into a small helper (e.g. `fn update_software_cursor(&mut self, target: ViewportPosition, responses: &mut VecDeque<Message>)` that computes the delta from `self.mouse_position`) and call it from both paths so the two copies cannot drift apart again.</comment>
<file context>
@@ -540,6 +540,17 @@ impl MessageHandler<TransformLayerMessage, TransformLayerMessageContext<'_>> for
let old_ptz = self.ptz;
self.ptz = document.document_ptz;
if old_ptz != self.ptz {
+ if self.software_cursor_active {
+ let delta = mouse_position - self.mouse_position;
+ self.software_cursor_pos += delta;
</file context>
| self.ptz = document.document_ptz; | ||
| if old_ptz != self.ptz { | ||
| self.mouse_position = input.mouse.position; | ||
| if self.software_cursor_active { |
There was a problem hiding this comment.
P3: When PTZ changes mid-gesture (e.g., wheel-zoom while dragging), this branch moves the software cursor by delta but the early return skips the G/R/S arms, so the object never gets that delta. The cursor (and the final hand-off of the grab) then stays offset from the transformed layer by the dropped deltas. Since the pre-existing code dropped the delta entirely on PTZ change, it either needs to apply the delta to the G/R/S operation as well (moving the return to after the transform arms), or not move the cursor here — otherwise the two paths diverge.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs, line 543:
<comment>When PTZ changes mid-gesture (e.g., wheel-zoom while dragging), this branch moves the software cursor by `delta` but the early `return` skips the G/R/S arms, so the object never gets that `delta`. The cursor (and the final hand-off of the grab) then stays offset from the transformed layer by the dropped deltas. Since the pre-existing code dropped the delta entirely on PTZ change, it either needs to apply the delta to the G/R/S operation as well (moving the return to after the transform arms), or not move the cursor here — otherwise the two paths diverge.</comment>
<file context>
@@ -540,6 +540,17 @@ impl MessageHandler<TransformLayerMessage, TransformLayerMessageContext<'_>> for
let old_ptz = self.ptz;
self.ptz = document.document_ptz;
if old_ptz != self.ptz {
+ if self.software_cursor_active {
+ let delta = mouse_position - self.mouse_position;
+ self.software_cursor_pos += delta;
</file context>
Closes #3255
Wrap cursor around viewport during G/R/S. While grabbing/rotating/scaling, hide OS cursor and show a Graphite fake that wraps within viewport bounds. Uses relative pointer-lock deltas for infinite drag on desktop and web. Works on Wayland where OS warp is not supported.
I have also add tests.
Like Blender GHOST_kGrabWrap.
web
https://github.com/user-attachments/assets/e8729966-7528-44b2-aa9f-5e08a74483ed
Desktop
https://github.com/user-attachments/assets/80f2b967-6ca5-444e-966a-8879eb7f6bef